-
-
Notifications
You must be signed in to change notification settings - Fork 44
feat: use JSON, templates, npx, and add dry-run support #186
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: use JSON, templates, npx, and add dry-run support #186
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR performs a major refactoring to improve the developer experience by migrating from INI/env files to JSON configurations, replacing custom templating with Mustache, adding dry-run support, and enabling npx-style usage. The primary goal is to simplify configuration management and reduce maintenance overhead.
- Migrates configuration from templates/ directory with custom parsing to structured JSON configs/ directory
- Replaces custom template system with industry-standard Mustache templating
- Adds --dry-run flag for testing without creating artifacts
Reviewed Changes
Copilot reviewed 117 out of 142 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
src/index.mjs | New main entry point with Commander.js CLI and dry-run support |
template.hbs | Single Mustache template replacing multiple custom templates |
configs/*.json | JSON configuration files replacing template-based config system |
src/meeting.mjs | Simplified meeting logic using JSON configs and Mustache |
package.json | Updated dependencies and entry points for npx support |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
819adb5
to
37d1262
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 117 out of 142 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/config.mjs:1
- The comments reference outdated configuration properties. The
meetingGroup
is now handled by commander insrc/index.mjs
, andgoogle.clientId
is not used anywhere in the codebase. These comments should be updated to reflect the actual usage.
/**
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
6436c01
to
0ea464b
Compare
@avivkeller apologies, I forgot to ask if you could rebase? |
0ea464b
to
4fcb8a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 117 out of 142 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 117 out of 142 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
); | ||
return obj; | ||
}, {}); | ||
Object.entries( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this was introduced in another PR, but could you please cleanup this function? It is too complex. The entries + reduce + map + complx split.slice.join.push is just too much.
// Search for events in the specified calendar using the filter text | ||
const response = await calendarClient.events.list({ | ||
calendarId: meetingConfig.properties.CALENDAR_ID?.replace(/"/g, ''), | ||
calendarId: `${calendar.id}@group.calendar.google.com`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LFX doesn't use Google Calendar, so we need to support non-Google calendar too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To my understanding that is just a manual replica that @bensternthal keeps updating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah for ow we are manually updating since we have some events in google calendar and some in LFX. We will be moving everything at some point to LFX. LFX does have ICAL feeds etc so folks can still add the calendar to whatever calendar they chose to use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah for ow we are manually updating since we have some events in google calendar and some in LFX. We will be moving everything at some point to LFX. LFX does have ICAL feeds etc so folks can still add the calendar to whatever calendar they chose to use.
Can you link the ICAL feed? We can update the script to use it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, here are some relevant links:
- https://zoom-lfx.platform.linuxfoundation.org/meetings/ojsf?view=week Public Calendar For All Of OpenJS
- https://webcal.prod.itx.linuxfoundation.org/lfx/a0941000002wBygAAE Public Ical Feed For All Of OpenJS
- https://zoom-lfx.platform.linuxfoundation.org/meetings/ojsf-nodejs?view=week Public Calendar Just For Node
- https://webcal.prod.itx.linuxfoundation.org/lfx/a092M00001IV4HSQA1 Public Ical Feed Just For Node
|
||
const program = new Command(); | ||
program | ||
.argument('[group]', 'Meeting group', 'tsc') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't have a default. Should fail if not provided IMO.
); | ||
|
||
// Create HackMD document with meeting notes and tags | ||
let hackmdNote; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the use of let here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could simply use a ternary
const issueContent = mustache.render(template, templateContext); | ||
|
||
// Create the actual issue | ||
let githubIssue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ternary instead of Let
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these dry run if statements are bloaty. We could simply have one big if
if (dryRun) {
....
do all these things
return;
}
normal operation
│ ├── github.mjs # GitHub API integration | ||
│ ├── google.mjs # Google APIs integration | ||
│ ├── meeting.mjs # Meeting operations | ||
| ├── index.mjs # Main application |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about configs folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I'm a fan of completely removing Markdown capabilities and all the content coming from a JSON file.
Surely a config JSON format is cool and standardised, but I should still be able to add whatever Markdown content I want as extra content and surely doing that via JSON ain't practical. It feels this PR is moving one step forward and two backwards.
Okay, let's break this up into several PRs, so each "step" can be independent. I'll break this up today into:
|
Fixes #3
Closes #185
This PR makes a few substantial changes to improve the DX of this tool.
templates/
to JSON configurations inconfigs/
, this'll make it easier to edit and parse (See fix(properties): allow quote-less props #185 for an example of dotenv struggles)--dry-run
argument, as, when testing this, it's important not to create the issues and notesnpx
-style support. (i.e.npx . tsc
ornpx https://github.com/nodejs/create-node-meeting-artifacts tsc
src/
, since it no longer really needs to be easily accessible for scripts, it'll benpx
-ed.The goal here is:
For example, running
npx . web --dry-run
produces: